Codecov Report

Merging #4958 (1c264e8) into main (0c4f065) will increase coverage by 0.10%.
The diff coverage is 95.95%.

@@            Coverage Diff             @@
##             main    #4958      +/-   ##
==========================================
+ Coverage   83.24%   83.35%   +0.10%     
==========================================
  Files         294      296       +2     
  Lines       12321    12301      -20     
==========================================
- Hits        10257    10253       -4     
+ Misses       2064     2048      -16     

How would you feel about names like SendToOpenTelemetry(...) or UseOpenTelemetry(...) for all the signals? My worry with AddOpenTelemetryMetrics() is that it sounds like OpenTelemetry is adding new metrics instrumentation rather than adding a new sink for the data.

@noahfalk I'm happy to name it whatever you want! If we do SendToOpenTelemetry or UseOpenTelemetry would we also add the same for ILoggingBuilder so it is consistent?

@reyang Had an interesting idea I want to also capture. IIRC, it was to recommend users use the "services.AddOpenTelemetry().With*" pattern for their pipeline configuration:

appBuilder.Logging.AddOpenTelemetry(); // Turn on ILogger integration
appBuilder.Metrics.AddOpenTelemetry(); // Turn on IMetricsListener integration

// Current style: Configure the pipeline
appBuilder.Services.AddOpenTelemetry()
   .WithLogging(logging => logging.AddOtlpExporter()
   .WithMetrics(metrics => metrics.AddOtlpExporter();

// Future style: Configure the pipeline using cross-cutting helper (https://github.com/open-telemetry/opentelemetry-dotnet/issues/4940)
appBuilder.Services.AddOpenTelemetry().AddOtlpExporter();

If we do SendToOpenTelemetry or UseOpenTelemetry would we also add the same for ILoggingBuilder so it is consistent?

Yeah, I was assuming we'd use the same name across logging + metrics (+distributed tracing eventually)

The other question which @samsp-msft raised that would good to look at is defaults. Effectively if an app author writes this and nothing else:

services.AddOpenTelemetry().WithMetrics()

... does it include IMetricsBuilder metrics by default or do they have to explicitly also call metricsBuilder.UseOpenTelemetry() to make that happen?

Certainly having it happen automatically seems nice in the vast majority of cases I can think of. The only case where it feels a little weird is:

1. Start with an app that already has a WithMetrics() call using an older version of OTel SDK + metrics configured in appsettings.json. This means the metrics aren't being sent to OTel despite being configured.
2. Upgrade that app to a newer version of OTel and now the appsettings.json metrics would start getting sent with no code changes.

That case feels fairly contrived so I'm leaning that yes the IMetricsBuilder metrics would be included by default when calling WithMetrics(). If we did it for metrics I think we'd also want to do it for logging (and distributed tracing in time) so that they are all consistent. Thoughts on this one?

The other question which @samsp-msft raised that would good to look at is defaults. Effectively if an app author writes this and nothing else:

services.AddOpenTelemetry().WithMetrics()

... does it include IMetricsBuilder metrics by default or do they have to explicitly also call metricsBuilder.UseOpenTelemetry() to make that happen?

Certainly having it happen automatically seems nice in the vast majority of cases I can think of.

Agreed. As this also requires changing config, there should be no changes to the app until they start adding metrics via config.

The only case where it feels a little weird is:

1. Start with an app that already has a WithMetrics() call using an older version of OTel SDK + metrics configured in appsettings.json. This means the metrics aren't being sent to OTel despite being configured.
2. Upgrade that app to a newer version of OTel and now the appsettings.json metrics would start getting sent with no code changes.

There have to be 2 changes here - updating the SDK and adding config.

That case feels fairly contrived so I'm leaning that yes the IMetricsBuilder metrics would be included by default when calling WithMetrics(). If we did it for metrics I think we'd also want to do it for logging (and distributed tracing in time) so that they are all consistent. Thoughts on this one?

Next - can we do away with WithMetrics()? Can there be an extension methods off the return from AddOpenTelemetry() that enable me to configure an exporter for all 3 sources. And can we bring logging into the fold so its configured the same way?

@noahfalk @samsp-msft Let's try to nail down the names here and get the initial support completed.

For the cross-cutting extension I have #4940 and for the default behavior I just opened #4985 we can tackle those things separately after this groundwork is completed?

Let's try to nail down the names here and get the initial support completed.

Sure. I'll narrow my proposal down to:

IMetricsBuilder.UseOpenTelemetry()
ILoggingBuilder.UseOpenTelemetry()

If everyone is happy with that we can go with it, if not we can workshop other names.

For the defaults I don't think it has to be this PR, but it do think it should be part of the same OTel SDK release that adds support for WithLogging() and IMetricsBuilder.

FYI I updated the code and the proposal in the description for "UseOpenTelemetry"